Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Support for enabling metrics in OpenTelemetry Collector #529

Merged
merged 3 commits into from
Sep 28, 2023

Conversation

SpiritZhou
Copy link
Contributor

Update a chart helm for new experimental features

  • add opentelemetry.operator.enabled
  • add opentelemetry.operator.otlpHTTPEndpoint

Checklist

  • I have verified that my change is according to the deprecations & breaking changes policy
  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • README is updated with new configuration values (if applicable) learn more
  • A PR is opened to update KEDA core (repo) (if applicable, ie. when deployment manifests are modified)

@zroubalik
Copy link
Member

@SpiritZhou it is not needed to release a new chart version. Just add the otel support to resources.

We will ship new Charts version during the release.

@tomkerkhove tomkerkhove changed the title Packaged new Helm chart version feat: Support for enabling metrics in OpenTelemetry Collector Sep 28, 2023
# -- Enable KEDA Operator opentelemetry metrics expose
enabled: false
# -- Set OTEL_EXPORTER_OTLP_ENDPOINT for opentelemetry exporter
otlpHTTPEndpoint: ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally think it would be better to use opentelemetry.collector.uri or opentelemetry.collector.uri.http since this would be the same for all components ond otlp is not something we should add to the name

keda/values.yaml Outdated Show resolved Hide resolved
keda/values.yaml Outdated Show resolved Hide resolved
@@ -139,6 +143,10 @@ spec:
- name: KEDA_RESTRICT_SECRET_ACCESS
value: {{ .Values.permissions.operator.restrict.secret | quote }}
{{- end }}
{{- if .Values.opentelemetry.operator.otlpHTTPEndpoint }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should only add it, if it's enabled as well

@@ -87,6 +87,10 @@ spec:
- "--webhooks-service-name={{ .Values.webhooks.name }}"
{{- if .Values.prometheus.operator.enabled }}
- --metrics-bind-address=:{{ .Values.prometheus.operator.port }}
- --enable-prometheus-metrics=:{{ .Values.prometheus.operator.enabled }}
{{- end }}
{{- if .Values.opentelemetry.operator.enabled }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's enabled but no collector URI was specified then we should fail

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomkerkhove tomkerkhove changed the base branch from main to otel-chart September 28, 2023 17:24
@tomkerkhove tomkerkhove marked this pull request as ready for review September 28, 2023 17:25
@tomkerkhove tomkerkhove requested a review from a team as a code owner September 28, 2023 17:25
@tomkerkhove tomkerkhove merged commit 587d0d5 into kedacore:otel-chart Sep 28, 2023
4 checks passed
tomkerkhove added a commit that referenced this pull request Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants